Repository navigation
Conversation
Freax13
reviewed
Oct 11, 2026
Freax13
left a comment
Freax13
left a comment
Member
There was a problem hiding this comment.
Thank you for your contribution!
| Page::range(start, end).into_iter().next(); | ||
| } | ||
|
|
||
| // TODO: This probably shouldn't panic, but we can't fix this without a breaking change. |
Member
There was a problem hiding this comment.
Please add this behavior change as a breaking change to the release notes.
|
|
||
| #[test] | ||
| #[should_panic = "attempt to add with overflow or resulted in non-canonical virtual address"] | ||
| fn test_page_range_next_jumping_gap_panics() { |
Comment on lines
+837
to
+840
| assert_eq!( | ||
| Page::range_inclusive(start, end).into_iter().next(), | ||
| Some(start) | ||
| ); |
Member
There was a problem hiding this comment.
We should add similar asserts to the remaining tests that formerly panicked.
Comment on lines
+220
to
+228
| // FIXME: Move this into the `Step` impl, once `Step` is stabilized. | ||
| pub(crate) fn backward_checked_impl(start: Self, count: usize) -> Option<Self> { | ||
| let count = u64::try_from(count).ok()?.checked_mul(S::SIZE)?; | ||
| let start_address = VirtAddr::backward_checked_u64(start.start_address, count)?; | ||
| Some(Self { | ||
| start_address, | ||
| size: PhantomData, | ||
| }) | ||
| } |
Member
There was a problem hiding this comment.
Let's replace the implementation of backward_checked for Page's Step with a call to this function.
Comment on lines
672
to
679
| // If the start of the inclusive range is 0, decrementing end until | ||
| // it is smaller than the start will cause an integer underflow. | ||
| // So instead, in that case we increment start rather than decrementing end. | ||
| if self.0.end.start_address().as_u64() != 0 { | ||
| self.0.end -= 1; | ||
| self.0.end = Page::backward_checked_impl(self.0.end, 1)?; | ||
| } else { | ||
| self.0.start += 1; | ||
| self.0.start = Page::forward_checked_impl(self.0.start, 1)?; | ||
| } |
Member
There was a problem hiding this comment.
Let's do this:
// If the start of the inclusive range is 0, decrementing end until
// it is smaller than the start will cause an integer underflow.
// In that case we increment start rather than decrementing end.
if let Some(end) = Page::backward_checked_impl(self.0.end, 1) {
self.0.end = end;
} else {
self.0.start = self.0.start + 1;
}
Comment on lines
+862
to
+869
| assert_eq!( | ||
| Page::range_inclusive(start, end).into_iter().next(), | ||
| Some(start) | ||
| ); | ||
| assert_eq!( | ||
| Page::range_inclusive(start, end).into_iter().next(), | ||
| Some(start) | ||
| ); |
Member
There was a problem hiding this comment.
There's a small issue in the original code: We called Page::range_inclusive(start, end).into_iter().next(); twice with the same inputs. We don't need to do this twice.
mkroening
force-pushed
the
range-jump-gap
branch
from
October 11, 2026 13:29
15f1675 to
fd0d920
Compare
Note that this does not add a checked version for subtracting two addresses yet.
Note that this changes the panic behavior, since `rhs * S::SIZE` did not panic before without overflow checks. Note that this does not add a checked version for subtracting two pages or frames yet.
mkroening
force-pushed
the
range-jump-gap
branch
from
October 11, 2026 13:30
fd0d920 to
d9c073c
Compare
Member
Author
|
Thanks for the review! All comments should be addressed. :) |
mkroening
force-pushed
the
range-jump-gap
branch
from
October 11, 2026 13:31
d9c073c to
62ace8a
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I recently discovered that this range panics during iteration, even though it does not even cross the address space gap:
We could use #616 to fix that, similar to #617. I believe it makes more sense to jump the gap, as proposed in #589 and already done via the unstable
Steptrait.